Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Generate event specific reports #32

Merged
merged 23 commits into from
Sep 16, 2023
Merged

✨ Generate event specific reports #32

merged 23 commits into from
Sep 16, 2023

Conversation

ubmarco
Copy link
Member

@ubmarco ubmarco commented Aug 26, 2023

This PR makes it possible to aggregate the runtime of event listeners for all fired Sphinx events.
Tricky part in pyinstrument is that all event listeners appear under EventManager.emit, so the event name is lost.
The PR monkeypatches Sphinx to use a custom Event listener which injects an event specific emit function, so it can be tracked in pyinstrument.

The PR can be tested with sphinx-analysis --project needs --pyinstrument --tree.
A new JSON report of name pyinstrument_sphinx_events.json is created.
It contains all unique combinations of file path, class and function of all event listeners, so it will pick up all extensions that have listeners registered.
The dictionary sphinx_performance/sphinx_events.py:CUSTOM_FRAMES_BY_EVENT can be modified to add more frames of interest (unrelated to events). It is currently used to track the runtime of sphinxcontrib-plantuml.

The generated JSON file can be used for 2 purposes:

  • As a support to the pyinstrument HTML report to get a per-event aggregation
  • To run in a CI and check how a PR to Sphinx or an extension affects performance

The HTML report now contains event specific function names, so event names can be searched.

@ubmarco ubmarco requested review from danwos and chrisjsewell August 26, 2023 22:37
@danwos
Copy link
Member

danwos commented Aug 28, 2023

I get an error during report generation when I run sphinx-analysis --project needs --pyinstrument --tree:

  File "/home/daniel/workspace/sphinx/sphinx-performance/sphinx_performance/analysis.py", line 268, in cli_analysis
    with Path.open("pyinstrument_profile.html", "w") as events_json_file:
  File "/usr/lib/python3.10/pathlib.py", line 1119, in open
    return self._accessor.open(self, mode, buffering, encoding, errors,
AttributeError: 'str' object has no attribute '_accessor'

@ubmarco
Copy link
Member Author

ubmarco commented Sep 1, 2023

I can confirm this bug for Python 3.10. For Python 3.11 it works. Need to check.

@ubmarco
Copy link
Member Author

ubmarco commented Sep 1, 2023

I fixed that problem, I need to instantiate a Path to be consumed by Path.open

sphinx_performance/analysis.py Outdated Show resolved Hide resolved
@ubmarco ubmarco requested a review from chrisjsewell September 1, 2023 13:35
@ubmarco ubmarco changed the title Generate event specific reports ✨ Generate event specific reports Sep 1, 2023
@ubmarco ubmarco changed the title ✨ Generate event specific reports ✨ Generate event specific reports Sep 1, 2023
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers, yeh I guess ideally it would be nice to have some tests 😅
But as there were already no tests for this package, I won't block on it

@ubmarco ubmarco merged commit 7d4483b into main Sep 16, 2023
2 checks passed
@ubmarco ubmarco deleted the mh-event-test branch September 16, 2023 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants